-
-
Notifications
You must be signed in to change notification settings - Fork 559
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
XWIKI-10871: Misalignments in the class editor on mobile #3885
base: master
Are you sure you want to change the base?
Conversation
* Added classes and changed the HTML structure so that each input field breaks together with its label
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general this looks good, thank you! Just a few small comments.
#end | ||
</select> | ||
<div id="add_xproperty" class="add_xproperty"> | ||
<div class="xeditor-input-group"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me like this class isn't used, and it seems to be new. Do we really need to introduce this new class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not necessary, I added it for future use. If you think it's better, I'll remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted in 6a4a4a6
<option value="${prop.name}">${escapetool.xml($prop.prettyName)}</option> | ||
#end | ||
</select> | ||
<div id="add_xproperty" class="add_xproperty"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be sufficient to target this element with the existing ID attribute? Other CSS rules in the same file targeting the same element use this ID, too. Or is there any reason to prefer using classes? If we introduce a new class, I would prefer to follow our naming conventions with add-xproperty
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd leave IDs for JS reference only, or as much as possible. My reason is that, while it's possible to target it with CSS, the specificities of IDs are higher than classes. See: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_cascade/Specificity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a commonly used pattern to not use IDs in CSS? I'm asking because in XWiki, I think we're currently not following this pattern. So maybe there should be a proposal to add this to our code style? To me, it seems quite inconsistent to have a CSS file with selectors for both the ID and the class that are both only on a single element. I'm not asking to completely change them, but to me the only good reason to have this inconsistency and not use the ID as in the rest of the CSS would be that we say that IDs shouldn't be used in CSS, and therefore we migrate our code to the new style whenever we touch it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a commonly used pattern to not use IDs in CSS?
It's very common, the lower the specificity of each CSS rule means that overwriting it will be easier for different cases like hovering, disabling for example. Also, there's better portability (defining one rule and using it across many different places).
To me, it seems quite inconsistent to have a CSS file with selectors for both the ID and the class that are both only on a single element.
You have a good point here. If you think it's more coherent, I'll port this implementation to use the ID, but open a proposal to change the code style. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with either using the ID or introducing a new class (but preferably using kebab-case, so add-xproperty
), but for the second option there would need to be a proposal to add this to the code style. You could open the proposal, and then we wait a few days, and if nobody objects, the PR can be merged.
@@ -307,6 +307,12 @@ The default state is collapsed for xobjects and xproperties, but expanded for xc | |||
background-image: url("$xwiki.getSkinFile('icons/datamodel/password.png')"); | |||
} | |||
|
|||
.add_xproperty { | |||
display: flex; | |||
gap: 8px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do these 8px come from? Wouldn't it be better to use something like 1em
to ensure that this spacing scales with the font size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a small value, to separate two elements, it could be changed to EM or REM for sure. Personally, I prefer REM since it's more predictable (REM stands for Root EM, so it's not compounding with other elements). Let me know if that's ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, the important part is to have some "rule" how much space we insert for such "small" spaces. Otherwise, we'll have a mix of hundreds of different spacing values in XWiki.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted to EMs on 6a4a4a6. As far as I know we don't have variables for spacing, at least looking at other components is pretty common finding values declared in PX.
In Cristal we do have variables for spacing, but to incorporate these in XS would be a big refactor of some components. Not impossible, of course but outside the scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the approach that we take to refactoring is usually to just introduce whatever new thing we need, and then to start using it whenever we change some code. Of course, this doesn't always work, and if possible it is also good to systematically adapt everything for the new thing, but in many cases that's just not feasible for a project of the size of XWiki.
In any case, I think 1em
is good, I think it makes sense to use the current font size as I assume that the spacing should really depend on the current font size and not the root font size.
* Changed px unit to em * Removed unused CSS class declaration
Jira URL
https://jira.xwiki.org/browse/XWIKI-10871
Changes
Description
dataeditors.css
andeditclass.vm
so that each input field break together with its label on smaller screens.Clarifications
Screenshots & Video
Screen.Recording.2025-02-10.at.11.26.06.mp4
Executed Tests
mvn clean install -f xwiki-platform-core/xwiki-platform-flamingo/xwiki-platform-flamingo-skin/xwiki-platform-flamingo-skin-resources
Expected merging strategy